Skip to content

feat(notebook-migration-service): compute jupyter iframe url per request - #7602

Merged
mengw15 merged 6 commits into
apache:mainfrom
zyratlo:migration-tool-stateless-backend
Aug 14, 2026
Merged

feat(notebook-migration-service): compute jupyter iframe url per request#7602
mengw15 merged 6 commits into
apache:mainfrom
zyratlo:migration-tool-stateless-backend

Conversation

@zyratlo

@zyratlo zyratlo commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Makes notebook-migration-service stateless so it can later run as a single global instance instead of one instance per user. This is the first backend stage of moving the service onto Texera's "orchestrator services are global, stateful resources are per user" pattern.

Today the service keeps a shared @volatile jupyterIframeURL: set-notebook writes it and get-jupyter-iframe-url reads it back. That shared state is only safe because each user happens to run their own pod, and even within one user it lets two browser tabs race. This PR removes the shared state and builds the URL from the request instead.

NotebookMigrationResource.scala

  • Removes the @volatile var jupyterIframeURL singleton and the warning comment that documented its per-user-pod assumption. Adds a defaultNotebookName constant (notebook.ipynb).
  • getJupyterIframeURL now takes a notebookName argument and builds the URL on each call. The name is validated with the same plain .ipynb regex setNotebook uses, since it now flows straight into the returned URL (blocks path traversal). The argument defaults to defaultNotebookName.
  • setNotebook no longer mutates any shared state; the assignment that wrote the singleton is gone. Its upload behavior is unchanged.
  • The /get-jupyter-iframe-url endpoint accepts an optional notebookName query parameter and falls back to the default when it is absent.

The change is backward compatible. The existing frontend calls the endpoint with no query parameter, which resolves to notebook.ipynb, exactly the URL it received before. No frontend, config, or deployment change is needed in this PR, and no other service or branch consumes the removed state.

Any related issues, documentation, discussions?

Closes #7390
Parent-issue #4301

How was this PR tested?

Extends the existing suite in NotebookMigrationResourceSpec.scala:

  • Fixed the two call sites that pass through the new endpoint signature.
  • Added a test that an explicit notebookName is honored in the returned URL.
  • Added a test that an invalid notebookName is rejected with 400 before any Jupyter call.
  • Added a test pinning the refactor: after setNotebook uploads other.ipynb, a parameter-less getJupyterIframeURL returns the default notebook.ipynb, proving the result no longer depends on state left by setNotebook.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 4.8)

@github-actions github-actions Bot added the platform Non-amber Scala service paths label Aug 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • No candidates found from git blame history.

@codecov-commenter

codecov-commenter commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.39%. Comparing base (eefd860) to head (bf9035e).
⚠️ Report is 18 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7602      +/-   ##
============================================
- Coverage     89.62%   89.39%   -0.24%     
+ Complexity     4395     4375      -20     
============================================
  Files          1177     1177              
  Lines         46884    46835      -49     
  Branches       5239     5225      -14     
============================================
- Hits          42022    41868     -154     
- Misses         3109     3220     +111     
+ Partials       1753     1747       -6     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø)
agent-service 98.62% <ø> (ø) Carriedforward from 96889a6
amber 85.74% <ø> (-0.64%) ⬇️ Carriedforward from 96889a6
computing-unit-managing-service 72.46% <ø> (ø)
config-service 77.31% <ø> (ø)
file-service 68.90% <ø> (ø)
frontend 90.57% <ø> (-0.02%) ⬇️ Carriedforward from 96889a6
notebook-migration-service 79.31% <100.00%> (+0.41%) ⬆️
pyamber 97.57% <ø> (ø) Carriedforward from 96889a6
workflow-compiling-service 57.89% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zyratlo
zyratlo marked this pull request as ready for review August 13, 2026 17:53
@zyratlo

zyratlo commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

/request-review @mengw15

@github-actions
github-actions Bot requested a review from mengw15 August 13, 2026 17:53
@mengw15
mengw15 requested a balanced review from Copilot August 14, 2026 03:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Makes notebook iframe URL generation stateless by deriving it from each request instead of shared process state.

Changes:

  • Removes the process-global iframe URL state.
  • Adds optional, validated notebook-name selection with a backward-compatible default.
  • Adds tests for default, explicit, invalid, and stateless URL generation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
NotebookMigrationResource.scala Computes validated iframe URLs per request and removes shared state.
NotebookMigrationResourceSpec.scala Tests explicit, invalid, default, and stateless iframe URL behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@mengw15 mengw15 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three comments. Copilot's three all hold up as well — the spec one especially: the positive test goes through the companion object, so the new query-param plumbing is the one part with no coverage.

@zyratlo zyratlo changed the title refactor(notebook-migration-service): compute jupyter iframe url per request feat(notebook-migration-service): compute jupyter iframe url per request Aug 14, 2026

@mengw15 mengw15 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The added comment plus #7665 covers what I was worried about — the constraint now has a home in both the code and the tracker. Deferring the frontend filename change until #7601 lands makes sense; worth a sub-issue for it, since this PR closes and the note goes with it.

@mengw15
mengw15 added this pull request to the merge queue Aug 14, 2026
Merged via the queue into apache:main with commit d94581a Aug 14, 2026
30 checks passed
@zyratlo

zyratlo commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

LGTM. The added comment plus #7665 covers what I was worried about — the constraint now has a home in both the code and the tracker. Deferring the frontend filename change until #7601 lands makes sense; worth a sub-issue for it, since this PR closes and the note goes with it.

I have raised issue #7671 to keep track of the frontend change

renovate-bot pushed a commit to renovate-bot/apache-_-texera that referenced this pull request Aug 17, 2026
apache#7738)

### What changes were proposed in this PR?
Uploads each workflow's notebook to Jupyter under a per-workflow
filename instead of a single shared `notebook.ipynb`.

Before this change the frontend always uploaded to
`work/notebook.ipynb`. That was safe across users (each runs their own
pod) but not across one user's workflows: every workflow wrote to the
same file, so opening a second workflow overwrote the first, and because
nothing writes back from Jupyter, any edits made in the panel were lost.
Two tabs on different workflows also collided on the same file. This PR
keys the notebook file on the workflow id (`notebook_<wid>.ipynb`) so
each workflow has its own.

The backend already accepts this (from apache#7602): `get-jupyter-iframe-url`
takes an optional `notebookName` query param and `set-notebook` accepts
any `[A-Za-z0-9._-]+\.ipynb` name, which `notebook_<wid>.ipynb`
satisfies. No backend change is needed.

**`NotebookMigrationService`**
- Adds an exported `notebookFileName(wid)` helper (mirrors the existing
`notebookMappingKey`) that returns `notebook_<wid>.ipynb`, or the
default when there is no wid.
- `sendNotebookToJupyter(notebookData, notebookName)` takes the name
instead of hardcoding it.
- `getJupyterIframeURL(notebookName?)` sends the name as the
`notebookName` query param when given, and omits it otherwise so the
backend default still applies.

**`JupyterPanelService` (owns the name)**
- Adds a private `currentNotebookFileName()` that derives the filename
from the current workflow's wid, and uses it for both the upload and the
iframe fetch so the two can never derive different names.
- Adds a public `getJupyterIframeURLForWorkflow()` that the panel calls
to get the URL for the current workflow's notebook.

**`JupyterNotebookPanelComponent` (view)**
- Calls `jupyterPanelService.getJupyterIframeURLForWorkflow()` and drops
its now-unused direct dependency on `NotebookMigrationService`.

Because the upload and the iframe fetch both go through
`currentNotebookFileName()`, the file that is written and the file the
panel requests are always the same, and switching workflows produces a
distinct `notebook_<wid>.ipynb` rather than overwriting a shared one.

### Any related issues, documentation, discussions?
Closes apache#7671
Parent issue apache#4301

Follow-up: deleting a notebook now leaves its `notebook_<wid>.ipynb`
file in the Jupyter pod, since `deleteNotebookAndMapping` only removes
the database rows. This was self-limiting under the old single-file
scheme. Tracked in apache#7737

### How was this PR tested?
- `notebook-migration.service.spec.ts`: `notebookFileName` mapping, the
request body carrying the name on `sendNotebookToJupyter`, and the
`notebookName` query param being present when a name is given and absent
when it is not.
- `jupyter-panel.service.spec.ts`: the upload uses the wid-derived
filename, `getJupyterIframeURLForWorkflow` forwards that same filename
to the HTTP client, and the disabled-flag path returns null without any
HTTP call.
- `jupyter-notebook-panel.component.spec.ts`: the panel fetches its URL
through `getJupyterIframeURLForWorkflow`.

### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

platform Non-amber Scala service paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Notebook Migration] Refactor notebook-migration-service to be stateless

4 participants